release/19.x: [SLP]Check that operand of abs does not overflow before making it part of minbitwidth transformation #113146
Conversation
|
@llvm/pr-subscribers-llvm-transforms Author: None (llvmbot) ChangesRequested by: @alexey-bataev Full diff: https://github.com/llvm/llvm-project/pull/113146.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index ab2b96cdc42db8..746ba51a981fe0 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -15440,9 +15440,25 @@ bool BoUpSLP::collectValuesToDemote(
MaskedValueIsZero(I->getOperand(1), Mask, SimplifyQuery(*DL)));
});
};
+ auto AbsChecker = [&](unsigned BitWidth, unsigned OrigBitWidth) {
+ assert(BitWidth <= OrigBitWidth && "Unexpected bitwidths!");
+ return all_of(E.Scalars, [&](Value *V) {
+ auto *I = cast<Instruction>(V);
+ unsigned SignBits = OrigBitWidth - BitWidth;
+ APInt Mask = APInt::getBitsSetFrom(OrigBitWidth, BitWidth - 1);
+ unsigned Op0SignBits =
+ ComputeNumSignBits(I->getOperand(0), *DL, 0, AC, nullptr, DT);
+ return SignBits <= Op0SignBits &&
+ ((SignBits != Op0SignBits &&
+ !isKnownNonNegative(I->getOperand(0), SimplifyQuery(*DL))) ||
+ MaskedValueIsZero(I->getOperand(0), Mask, SimplifyQuery(*DL)));
+ });
+ };
if (ID != Intrinsic::abs) {
Operands.push_back(getOperandEntry(&E, 1));
CallChecker = CompChecker;
+ } else {
+ CallChecker = AbsChecker;
}
InstructionCost BestCost =
std::numeric_limits<InstructionCost::CostType>::max();
diff --git a/llvm/test/Transforms/SLPVectorizer/abs-overflow-incorrect-minbws.ll b/llvm/test/Transforms/SLPVectorizer/abs-overflow-incorrect-minbws.ll
new file mode 100644
index 00000000000000..51b635837d3b59
--- /dev/null
+++ b/llvm/test/Transforms/SLPVectorizer/abs-overflow-incorrect-minbws.ll
@@ -0,0 +1,33 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S --passes=slp-vectorizer < %s | FileCheck %s
+
+define i32 @test(i32 %n) {
+; CHECK-LABEL: define i32 @test(
+; CHECK-SAME: i32 [[N:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: [[TMP0:%.*]] = insertelement <2 x i32> poison, i32 [[N]], i32 0
+; CHECK-NEXT: [[TMP1:%.*]] = shufflevector <2 x i32> [[TMP0]], <2 x i32> poison, <2 x i32> zeroinitializer
+; CHECK-NEXT: [[TMP2:%.*]] = add <2 x i32> [[TMP1]], <i32 1, i32 2>
+; CHECK-NEXT: [[TMP3:%.*]] = zext <2 x i32> [[TMP2]] to <2 x i64>
+; CHECK-NEXT: [[TMP7:%.*]] = mul nuw nsw <2 x i64> [[TMP3]], <i64 273837369, i64 273837369>
+; CHECK-NEXT: [[TMP8:%.*]] = call <2 x i64> @llvm.abs.v2i64(<2 x i64> [[TMP7]], i1 true)
+; CHECK-NEXT: [[TMP4:%.*]] = trunc <2 x i64> [[TMP8]] to <2 x i32>
+; CHECK-NEXT: [[TMP5:%.*]] = extractelement <2 x i32> [[TMP4]], i32 0
+; CHECK-NEXT: [[TMP6:%.*]] = extractelement <2 x i32> [[TMP4]], i32 1
+; CHECK-NEXT: [[RES1:%.*]] = add i32 [[TMP5]], [[TMP6]]
+; CHECK-NEXT: ret i32 [[RES1]]
+;
+entry:
+ %n1 = add i32 %n, 1
+ %zn1 = zext nneg i32 %n1 to i64
+ %m1 = mul nuw nsw i64 %zn1, 273837369
+ %a1 = call i64 @llvm.abs.i64(i64 %m1, i1 true)
+ %t1 = trunc i64 %a1 to i32
+ %n2 = add i32 %n, 2
+ %zn2 = zext nneg i32 %n2 to i64
+ %m2 = mul nuw nsw i64 %zn2, 273837369
+ %a2 = call i64 @llvm.abs.i64(i64 %m2, i1 true)
+ %t2 = trunc i64 %a2 to i32
+ %res1 = add i32 %t1, %t2
+ ret i32 %res1
+}
|
|
who can review this? @nikic? |
| ComputeNumSignBits(I->getOperand(0), *DL, 0, AC, nullptr, DT); | ||
| return SignBits <= Op0SignBits && | ||
| ((SignBits != Op0SignBits && | ||
| !isKnownNonNegative(I->getOperand(0), SimplifyQuery(*DL))) || |
There was a problem hiding this comment.
I don't understand this part of the condition. What is the meaning of SignBits != Op0SignBits && !isKnownNonNegative? Should this be SignBits != Op0SignBits || isKnownNonNegative?
Though I'm not really sure why we'd be interested in handling the case where the abs is known non-negative (including also the MaskedValueIsZero check below): If it's non-negative, wouldn't we expect the abs to fold away anyway?
There was a problem hiding this comment.
Right, isKnownNonNegative is meaningless here. Instead we check that the value is negative and SignBits < Op0SignBits, i.e. we do not lose signedness info when trying to truncate the operand value
There was a problem hiding this comment.
Should this then check isKnownNegative rather than !isKnownNonNegative? They're not the same. I'm still confused by this code.
There was a problem hiding this comment.
!isKnownNonNegative is correct here
|
What's the status of this one? Can it / should it be merged? |
|
Ping on this again. Are you happy with this now @nikic ? |
(cherry picked from commit 825f9cb)
…t of minbitwidth transformation Need to check that the operand of the abs intrinsic can be safely truncated before making it part of the minbitwidth transformation. Fixes llvm#112577 (cherry picked from commit 709abac)
|
@alexey-bataev (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR. |
4 participants
Backport 825f9cb 709abac
Requested by: @alexey-bataev